-
Notifications
You must be signed in to change notification settings - Fork 442
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Aggressively clean up the Protobuf CMake dependency. #4543
Conversation
5b54ad8
to
805b683
Compare
@jkhsjdhjs We are trying to refresh our Protobuf dependency and simplify it but I know you had some trouble with setting the right variables in Arch Protobuf. If we mandate a recent version do these build settings work for you? Or do we still need to keep the compatibility variables? |
b38b53d
to
ba10fba
Compare
@fruffy Yep, it doesn't build for me with the changes of this PR:
It works if I remove the However, I then get these errors:
|
Thanks for checking this! Maybe using |
Not using
Using |
ba10fba
to
baff211
Compare
0909166
to
2ad6ebe
Compare
82c482e
to
1196ce4
Compare
@jkhsjdhjs I tested this locally and on MacOS, and I believe things should now work. Took me a lot of trial and error. |
759b06d
to
112546d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Yeah, a lot of this was me banging my head against the wall trying to figure out why the MacOS build can not find Protobuf. Only to realize that it requires path hints. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two things I've noticed. (I got little lost in all the changes so I did not fully review it, but it looks reasonable overall).
8b4d5e5
to
0fb6e74
Compare
0fb6e74
to
de906e7
Compare
de906e7
to
9c38d64
Compare
91e1a5f
to
d78d418
Compare
d78d418
to
69cd45d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am no familiar with protobuf and how it should be used in CMake. Nor am I familiar with the P4C control plane. But I don't see any problems with the code.
fwiw I have been testing this PR in downstream dependencies for months now. |
Now that we are mandating Protobuf 3.25 try to simplify the use of extra variables and workarounds for our Protobuf dependency.
protobuf_generate
instead our own custom command. This way we can rely on the canonical definitions instead of having to maintain our own generators.protobuf::
PROTOBUF_PROTOC_INCLUDES
, instead useProtobuf_INCLUDE_DIRS
only.With CMake 3.24 we may also be able to override
find_package
with FetchContent: https://cmake.org/cmake/help/latest/module/FetchContent.htmlFixes #4477.